Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-124405: Fix NameError in openpty #124406

Merged
merged 1 commit into from
Sep 24, 2024
Merged

gh-124405: Fix NameError in openpty #124406

merged 1 commit into from
Sep 24, 2024

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 24, 2024

@ambv how can I test this change? This looks very platform specific, but it seems that you have a reproducer :)

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks like the correct inlining of slave_open from #118826. Adding more test coverage sounds good though!

@sobolevn
Copy link
Member Author

Adding more test coverage sounds good though!

Thanks for the review! I need help adding tests, since I cannot reproduce this on my machine :(

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to sleep now, will look tomorrow. Based on typeshed I_PUSH should exist on Linux?

@hauntsaninja
Copy link
Contributor

"highly problematic" indeed ;-) https://github.com/python/typeshed/pull/6807/files#diff-f28c4d3b27ed19db8e4154e55b1ad119f6655aad863bd3e92d9aee2d481589d7R72

@vstinner
Copy link
Member

I tried to test this change by removing os.openpty():

import os
import pty
del os.openpty
master_fd, slave_fd = pty.openpty()
os.close(master_fd)
os.close(slave_fd)

But it fails on Linux with:

Traceback (most recent call last):
  File "/home/vstinner/python/main/x.py", line 4, in <module>
    master_fd, slave_fd = pty.openpty()
                          ~~~~~~~~~~~^^
  File "/home/vstinner/python/main/Lib/pty.py", line 34, in openpty
    master_fd, slave_name = _open_terminal()
                            ~~~~~~~~~~~~~~^^
  File "/home/vstinner/python/main/Lib/pty.py", line 58, in _open_terminal
    raise OSError('out of pty devices')
OSError: out of pty devices

On my Fedora 41, I have no /dev/pty* device, only /dev/tty* devices (97 files) that I cannot open (PermissionError) because my user is not in tty or dialout groups.

It seems like os.openpty() is implemented by communicating with /dev/ptmx device using ioctl(). strace:

openat(AT_FDCWD, "/dev/ptmx", O_RDWR)   = 3
ioctl(3, TIOCGPTN, [3])                 = 0
ioctl(3, TIOCSPTLCK, [0])               = 0
ioctl(3, TIOCGPTPEER, 0x102)            = 4
ioctl(3, FIOCLEX)                       = 0
ioctl(4, FIOCLEX)                       = 0

@vstinner
Copy link
Member

On my Fedora 41, I have no /dev/pty* device, only /dev/tty* devices (97 files) that I cannot open (PermissionError) because my user is not in tty or dialout groups.

Same on FreeBSD. On FreeBSD, os.openpty() is implemented with (syscalls recorded by truss):

posix_openpt(O_RDWR|O_NOCTTY)                    = 3 (0x3) 
ioctl(3,TIOCPTMASTER,0x0)                        = 0 (0x0)
ioctl(3,TIOCPTMASTER,0x0)                        = 0 (0x0)
ioctl(3,TIOCPTMASTER,0x0)                        = 0 (0x0)
ioctl(3,FIODGNAME,0x820f8bb90)                   = 0 (0x0)
openat(AT_FDCWD,"/dev/pts/1",O_RDWR,00)          = 4 (0x4)
ioctl(3,FIOCLEX,0x0)                             = 0 (0x0)
ioctl(4,FIOCLEX,0x0)                             = 0 (0x0)

@vstinner
Copy link
Member

pty._open_terminal() code is quite old, it was added in 1994 when the pty module was created by commit 23cb2a8. The code almost didn't change since 1994: it still tries to open a /dev/ptyXY device and then use /dev/ttyXY device.

I guess that os.openpty() is now commonly available, and nobody uses the _open_terminal() fallback code path anymore, which looks broken on most platforms.

We should maybe do something with this code. Deprecate it. Remove it. I don't know :-)

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sobolevn
Copy link
Member Author

@vstinner thanks a lot for your help! 👍

@ambv ambv merged commit 17b3bc9 into python:main Sep 24, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants